feat(redshift): optionally reboot Clusters to apply parameter changes #22063

dontirun · 2022-09-15T19:51:47Z

Closes #22009

Currently waiting on #22055 and #22059 for the assertions in the integration test to successfully run

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-09-15T19:51:55Z

dontirun · 2022-09-15T19:53:26Z

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts

+ await redshift.rebootCluster({ ClusterIdentifier: clusterId }).promise();
+ } catch (err) {
+ if ((<any>err).code === 'InvalidClusterState') {
+ return await executeActionForStatus(status, 30000);


I thought about doing some sort of backoff here, but found checking every 30 seconds to be sufficient in my testing.

aws-cdk-automation · 2022-10-07T00:16:48Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

dontirun · 2022-10-07T00:22:25Z

I was waiting on #22059 to be fixed before updating. I have manually confirmed (by looking at the cluster in the integration tests) that this works.

… is creted

aws-cdk-automation · 2022-11-10T00:10:11Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

mrgrain · 2022-12-08T13:19:57Z

@dontirun The commented tests feel quite essential to this. I'm drafting this PR until you can get to it. Looks like a cool feature though. 🚀

dontirun · 2022-12-08T16:11:17Z

@mrgrain I sadly couldn't get a few of things working with integ-tests library.

The commented tests didn't work due to limitations with custom resource/lambda response size limitations (can't filter the response)
Diff assets should be enabled, but I kept getting different asset hashes in the codebuild report although it would verify locally

mrgrain · 2022-12-08T16:21:29Z

@mrgrain I sadly couldn't get a few of things working with integ-tests library.

Hmm, we still need a way to verify the custom resource works. 🤔

The commented tests didn't work due to limitations with custom resource/lambda response size limitations (can't filter the response)

The PR you've linked as been merged. Is something else missing?

Diff assets should be enabled, but I kept getting different asset hashes in the codebuild report although it would verify locally

Why does it need to be enabled? But yes, it's weird you get a different hash.

aws-cdk-automation · 2023-02-05T00:09:05Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-02-06T00:08:36Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-02-07T00:09:12Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-02-08T00:08:08Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-02-09T00:09:08Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

comcalvi

Very interesting feature! There's a few things I don't quite understand yet

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts

comcalvi · 2023-02-09T22:57:23Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ ParameterGroupName: Lazy.string({
+ produce: () => {
+ if (!this.parameterGroup) {
+ throw new Error('Cannot enable reboot for parameter changes when there is no associated ClusterParameterGroup.');


Putting implementation details aside I didn't think it would be a good user experience to allow for enabling a feature that does nothing.

Using a Lazy gets around an order of operations error where enabling the feature and then later adding a parameter group would trigger this error (this test case)

Pull request has been modified.

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts

comcalvi · 2023-02-14T19:07:30Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ /**
+ * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply.
+ */
+ protected rebootForParameterChangesEnabled?: boolean;


I don't think we need this prop, can we remove it? It seems redundant with rebootForParameterChanges?

Related to my other comment rebootForParameterChanges is the class property that is needed so we don't create duplicate custom resources with enableRebootForParameterChanges(). I thought it would be clearer to have this property versus making the Provider and CustomResource class properties

Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()

comcalvi · 2023-02-14T19:08:21Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ if (!this.rebootForParameterChangesEnabled) {
+ this.rebootForParameterChangesEnabled = true;


The check on line 607 is all we need for this, no?

I believe we need it to make sure we don't make a duplicate Custom Resource. I could create class variables to make the function idempotent, but thought that may be overkill

making the function idempotent is definitely overkill, and guarding against these potential bugs (an error would be thrown complaining about duplicate construct IDs) is definitely the right call in general. In this case though I don't see how the function could be called twice, as it's only called in the constructor and it's internal-only.

I originally thought about making it constructor only, but It's currently not (example). I figured that exposing the method would be friendlier for situationally enabling the feature under specific conditions (ex. beta/prod stages, using the addToParameterGroup method and then using this method).

hmmm okay. I don't love that we have this prop and this internal member, but it is a better user experience to be able to use a convenience method to set it, so we can leave it.

Please change the format of this to a guard clause though, eg:

if (this.rebootForParameterChangesEnabled) { return; } this.rebootForParameterChangesEnabled = true;

…boot-handler/index.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Pull request has been modified.

comcalvi · 2023-02-15T18:26:28Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ if (!this.rebootForParameterChangesEnabled) {
+ this.rebootForParameterChangesEnabled = true;


making the function idempotent is definitely overkill, and guarding against these potential bugs (an error would be thrown complaining about duplicate construct IDs) is definitely the right call in general. In this case though I don't see how the function could be called twice, as it's only called in the constructor and it's internal-only.

comcalvi · 2023-02-16T20:55:26Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ if (!this.rebootForParameterChangesEnabled) {
+ this.rebootForParameterChangesEnabled = true;


hmmm okay. I don't love that we have this prop and this internal member, but it is a better user experience to be able to use a convenience method to set it, so we can leave it.

Please change the format of this to a guard clause though, eg:

if (this.rebootForParameterChangesEnabled) { return; } this.rebootForParameterChangesEnabled = true;

comcalvi · 2023-02-16T20:56:04Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+ /**
+ * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply.
+ */
+ protected rebootForParameterChangesEnabled?: boolean;


Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()

Pull request has been modified.

dontirun · 2023-02-17T16:01:32Z

@comcalvi

I got around needing the rebootForParameterChangesEnabled internel member by using node.tryFindChild

    if (this.node.tryFindChild('RedshiftClusterRebooterCustomResource')) {
      return;
    }

comcalvi · 2023-02-17T21:55:37Z

The node.tryFindChild() solution is perfect! Nice work

mergify · 2023-02-17T21:55:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-02-17T23:13:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 16ba85f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-02-17T23:13:48Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

dontirun and others added 3 commits September 12, 2022 22:44

inital implementation

8b8798c

add lambda handler

68ebe28

documentation, tests, and bugfixes

f262a52

aws-cdk-automation requested a review from a team September 15, 2022 19:52

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Sep 15, 2022

dontirun commented Sep 15, 2022

View reviewed changes

dontirun changed the title ~~feat(redshift-alpha): optionally reboot Clusters to apply parameter changes~~ feat(redshift): optionally reboot Clusters to apply parameter changes Sep 20, 2022

Arun Donti added 2 commits September 30, 2022 10:55

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

6c1122b

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

c61f994

Arun Donti added 7 commits October 10, 2022 12:13

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

6fe9387

fix: allow for enabling the reboot feature before the parameter group…

63400f6

… is creted

test: comment out assertions

a5d1ee5

chore: commit deleted asstes

153345a

chore: update snapshot

f31b6dd

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

a01c334

tests: regenerate snapshot

52916e6

dontirun force-pushed the feat-reboot-cluster-for-parameter-updates branch from 157b2ac to 52916e6 Compare October 19, 2022 16:41

Arun Donti added 3 commits November 16, 2022 11:52

chore: commenting out diff assets in integ test

c445b0f

docs: add testing procedure to integ test

26fbbaa

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

b732dcf

mrgrain marked this pull request as draft December 8, 2022 13:20

comcalvi previously requested changes Feb 9, 2023

View reviewed changes

refactor: removing unknown-error from reboot actions

b6c323d

Merge branch 'master' into feat-reboot-cluster-for-parameter-updates

1c69185

dontirun requested a review from comcalvi February 14, 2023 16:35

comcalvi previously requested changes Feb 14, 2023

View reviewed changes

Update packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-re…

80848a9

…boot-handler/index.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

comcalvi requested changes Feb 15, 2023

View reviewed changes

comcalvi previously requested changes Feb 16, 2023

View reviewed changes

refactor: use guard clause for clarity

f91ffeb

refactor: update guard clause

a76b887

comcalvi approved these changes Feb 17, 2023

View reviewed changes

Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

16ba85f

mergify bot merged commit f61d950 into aws:main Feb 17, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redshift): optionally reboot Clusters to apply parameter changes #22063

feat(redshift): optionally reboot Clusters to apply parameter changes #22063

dontirun commented Sep 15, 2022

gitpod-io bot commented Sep 15, 2022

dontirun Sep 15, 2022

aws-cdk-automation commented Oct 7, 2022

dontirun commented Oct 7, 2022

aws-cdk-automation commented Nov 10, 2022

mrgrain commented Dec 8, 2022

dontirun commented Dec 8, 2022

mrgrain commented Dec 8, 2022

aws-cdk-automation commented Feb 5, 2023

aws-cdk-automation commented Feb 6, 2023

aws-cdk-automation commented Feb 7, 2023

aws-cdk-automation commented Feb 8, 2023

aws-cdk-automation commented Feb 9, 2023

comcalvi left a comment

comcalvi Feb 9, 2023

dontirun Feb 14, 2023

comcalvi Feb 14, 2023

dontirun Feb 14, 2023

comcalvi Feb 16, 2023

comcalvi Feb 14, 2023

dontirun Feb 14, 2023

comcalvi Feb 15, 2023

dontirun Feb 15, 2023

comcalvi Feb 16, 2023

comcalvi Feb 15, 2023

comcalvi Feb 16, 2023

comcalvi Feb 16, 2023

dontirun commented Feb 17, 2023 •

edited

Loading

comcalvi commented Feb 17, 2023

mergify bot commented Feb 17, 2023

aws-cdk-automation commented Feb 17, 2023

mergify bot commented Feb 17, 2023

		if (!this.rebootForParameterChangesEnabled) {
		this.rebootForParameterChangesEnabled = true;

feat(redshift): optionally reboot Clusters to apply parameter changes #22063

feat(redshift): optionally reboot Clusters to apply parameter changes #22063

Conversation

dontirun commented Sep 15, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 15, 2022

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 7, 2022

dontirun commented Oct 7, 2022

aws-cdk-automation commented Nov 10, 2022

mrgrain commented Dec 8, 2022

dontirun commented Dec 8, 2022

mrgrain commented Dec 8, 2022

aws-cdk-automation commented Feb 5, 2023

aws-cdk-automation commented Feb 6, 2023

aws-cdk-automation commented Feb 7, 2023

aws-cdk-automation commented Feb 8, 2023

aws-cdk-automation commented Feb 9, 2023

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dontirun commented Feb 17, 2023 • edited Loading

comcalvi commented Feb 17, 2023

mergify bot commented Feb 17, 2023

aws-cdk-automation commented Feb 17, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 17, 2023

dontirun commented Feb 17, 2023 •

edited

Loading